[AIRFLOW-5390] Remove provide context #5990

Fokko · 2019-09-03T08:04:08Z

Make sure you have checked all steps below.

I'm giving Apache Airflow training across Europe, and at every workshop that I provide, I find it a bit awkward to introduce the idea of the provide_context. Instead, I want to remove this thing and make it infer the variables automagically based on the signature of the Python callable. Less is more in terms of the public API :-)

Jira

My PR addresses the following Airflow Jira issues and references them in the PR title. For example, "[AIRFLOW-5390] My Airflow PR"
- https://issues.apache.org/jira/browse/AIRFLOW-5390
- In case you are fixing a typo in the documentation you can prepend your commit with [AIRFLOW-XXX], code changes always need a Jira issue.
- In case you are proposing a fundamental code change, you need to create an Airflow Improvement Proposal (AIP).
- In case you are adding a dependency, check if the license complies with the ASF 3rd Party License Policy.

Description

Here are some details about my PR, including screenshots of any UI changes:

Tests

My PR adds the following unit tests OR does not need testing for this extremely good reason:

Commits

My commits all reference Jira issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
1. Subject is separated from body by a blank line
2. Subject is limited to 50 characters (not including Jira issue reference)
3. Subject does not end with a period
4. Subject uses the imperative mood ("add", not "adding")
5. Body wraps at 72 characters
6. Body explains "what" and "why", not "how"

Documentation

In case of new functionality, my PR adds documentation that describes how to use it.
- All the public functions and the classes in the PR contain docstrings that explain what it does
- If you implement backwards incompatible changes, please leave a note in the Updating.md so we can assign it to a appropriate release

Code Quality

Passes flake8

…provide-context-removed

airflow/operators/python_operator.py

ashb

Interesting idea!

I'd like to see expanded tests on this, as I think this might break op_kwargs a bit. For example this would print 1 before, but now I think will print something else.

PythonOperator(
    op_kwargs={'dag': 1},
    python_callable: lambda dag: print(dag)
)

This needs a note in UPDATING.md too saying how it works.

I think if we leave (but ignore/warn about) provide_context we could make this mostly a non-breaking change which makes our users' lives easier when upgrading

ashb · 2019-09-03T09:45:01Z

Another example: what happens if I do:

def fn(dag, **context):
    print(dag)

PythonOperator(
    op_args=[1],
    python_callable=fn
)

Fokko · 2019-09-03T10:25:44Z

Thanks @ashb for thinking along. Appreciate it. I've added the tests to the suite.

I think we can fix the one with the op_args by skipping the first len(op_args) number of arguments. This would not introduce any change in behavior. Similar to the arguments in the kwargs, we could give the keys in op_kwargs priority.

kwargs is actually handled: https://github.com/apache/airflow/blob/master/airflow/operators/python_operator.py#L108-L109

…provide-context-removed

Fokko · 2019-09-04T13:11:15Z

Everything is green now

Changed the behavior. This following snippet will throw an exception now, since dag is a reserved keyword:

def fn(dag, **context):
    print(dag)

PythonOperator(
    op_args=[1],
    python_callable=fn
)

Otherwise it might become very confusing when you change the op_args etc.

…provide-context-removed

…ntext-removed

codecov-io · 2019-09-09T11:49:36Z

Codecov Report

Merging #5990 into master will decrease coverage by 0.1%.
The diff coverage is 94.44%.

@@            Coverage Diff             @@
##           master    #5990      +/-   ##
==========================================
- Coverage   80.02%   79.91%   -0.11%     
==========================================
  Files         594      594              
  Lines       34769    34794      +25     
==========================================
- Hits        27824    27807      -17     
- Misses       6945     6987      +42

Impacted Files	Coverage Δ
...ow/contrib/example_dags/example_qubole_operator.py	`0% <ø> (ø)`	⬆️
airflow/example_dags/docker_copy_data.py	`100% <ø> (ø)`	⬆️
airflow/gcp/utils/mlengine_operator_utils.py	`95.34% <ø> (ø)`	⬆️
airflow/example_dags/example_python_operator.py	`94.44% <ø> (ø)`	⬆️
...ample_dags/example_branch_python_dop_operator_3.py	`75% <ø> (ø)`	⬆️
airflow/example_dags/example_trigger_target_dag.py	`92.3% <ø> (ø)`	⬆️
airflow/example_dags/example_xcom.py	`62.5% <ø> (ø)`	⬆️
airflow/operators/python_operator.py	`95.75% <100%> (+0.33%)`	⬆️
airflow/contrib/sensors/python_sensor.py	`100% <100%> (+15%)`	⬆️
airflow/sensors/http_sensor.py	`96.66% <100%> (ø)`	⬆️
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d31c092...b592e68. Read the comment docs.

UPDATING.md

airflow/operators/python_operator.py

mik-laj · 2019-09-10T13:59:58Z

I am not sure if it has sufficient documentation. Could you check that the documentation in the docs/howto/operator/python.rst file is complete?

This is very well described in the UPDATING.md file, but it is not a good place to put valuable for first user documentation.

kaxil · 2019-09-12T11:26:24Z

Awesome work @Fokko

Fokko added 4 commits August 5, 2019 19:40

Remove provide context

0472c2d

Merge branch 'master' of github.com:apache/incubator-airflow into fd-…

7420ed5

…provide-context-removed

Merge branch 'master' of github.com:apache/incubator-airflow into fd-…

7bcdbe0

…provide-context-removed

Simplify the arguments

66ab4f7

BasPH reviewed Sep 3, 2019

View reviewed changes

airflow/operators/python_operator.py Outdated Show resolved Hide resolved

airflow/operators/python_operator.py Outdated Show resolved Hide resolved

ashb requested changes Sep 3, 2019

View reviewed changes

Fokko added 2 commits September 3, 2019 11:47

Feedback is een cadeautje

15b8eef

Add additional tests

6c0c8bb

Fokko added 4 commits September 3, 2019 12:44

Cover some edge cases

c6ba062

Extend the tests

d99bb9c

Merge branch 'master' of github.com:apache/incubator-airflow into fd-…

a06c8fb

…provide-context-removed

Update the tests

145f835

Fokko closed this Sep 4, 2019

Fokko added 7 commits September 4, 2019 11:08

Remove flake8 violations

d0be333

Works on my machine (using Breeze :-)

3fc2e98

Clean up occurrences with provide_context

c9995ee

Some cleanup

d7f35d6

Fix the types

7ab68bc

Less is more

2cde97b

Patch tests

95764ee

Fokko reopened this Sep 4, 2019

Fokko and others added 4 commits September 4, 2019 15:13

Merge branch 'master' into fd-provide-context-removed

bf85b84

Merge branch 'master' of github.com:apache/incubator-airflow into fd-…

79f23ef

…provide-context-removed

Merge branch 'master' of github.com:apache/incubator-airflow into fd-…

0ce92a6

…provide-context-removed

Merge branch 'master' of github.com:apache/airflow into fd-provide-co…

a5d99ed

…ntext-removed

BasPH reviewed Sep 10, 2019

View reviewed changes

UPDATING.md Outdated Show resolved Hide resolved

BasPH reviewed Sep 10, 2019

View reviewed changes

airflow/operators/python_operator.py Outdated Show resolved Hide resolved

airflow/operators/python_operator.py Outdated Show resolved Hide resolved

Feedback from Bas

b592e68

BasPH merged commit dd175fa into apache:master Sep 10, 2019

mik-laj mentioned this pull request Sep 10, 2019

[AIRFLOW-5390] Remove provide context #6074

Closed

5 tasks

This was referenced Sep 17, 2019

Update example_trigger_target_dag.py #6136

Closed

[AIRFLOW-5510] Add provide context #6137

Closed

mik-laj mentioned this pull request Mar 23, 2020

Fix example DAG for MLEngine in backport package #7813

Merged

5 tasks

mik-laj mentioned this pull request May 24, 2020

Fixed name of 20 remaining wrongly named operators. #8994

Merged

6 tasks

yuqian90 mentioned this pull request Oct 28, 2020

Extend the same keyword arguments callable support in PythonOperator to some other sensors/operators #11922

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AIRFLOW-5390] Remove provide context #5990

[AIRFLOW-5390] Remove provide context #5990

Fokko commented Sep 3, 2019 •

edited

Loading

ashb left a comment

ashb commented Sep 3, 2019

Fokko commented Sep 3, 2019 •

edited

Loading

Fokko commented Sep 4, 2019 •

edited

Loading

codecov-io commented Sep 9, 2019 •

edited

Loading

mik-laj commented Sep 10, 2019 •

edited

Loading

kaxil commented Sep 12, 2019

[AIRFLOW-5390] Remove provide context #5990

[AIRFLOW-5390] Remove provide context #5990

Conversation

Fokko commented Sep 3, 2019 • edited Loading

Jira

Description

Tests

Commits

Documentation

Code Quality

ashb left a comment

Choose a reason for hiding this comment

ashb commented Sep 3, 2019

Fokko commented Sep 3, 2019 • edited Loading

Fokko commented Sep 4, 2019 • edited Loading

codecov-io commented Sep 9, 2019 • edited Loading

Codecov Report

mik-laj commented Sep 10, 2019 • edited Loading

kaxil commented Sep 12, 2019

Fokko commented Sep 3, 2019 •

edited

Loading

Fokko commented Sep 3, 2019 •

edited

Loading

Fokko commented Sep 4, 2019 •

edited

Loading

codecov-io commented Sep 9, 2019 •

edited

Loading

mik-laj commented Sep 10, 2019 •

edited

Loading